Skip to content

Fix incorrect test code for error handling#27139

Merged
wxiaoguang merged 2 commits into
go-gitea:mainfrom
wxiaoguang:fix-wiki-test
Sep 20, 2023
Merged

Fix incorrect test code for error handling#27139
wxiaoguang merged 2 commits into
go-gitea:mainfrom
wxiaoguang:fix-wiki-test

Conversation

@wxiaoguang
Copy link
Copy Markdown
Contributor

@wxiaoguang wxiaoguang commented Sep 19, 2023

If assert.NoError fails, it should return. Otherwise the code could still execute even if the error occurs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 19, 2023
@delvh
Copy link
Copy Markdown
Member

delvh commented Sep 19, 2023

But if assert.NoError fails, the test will already be marked as failed, correct?

@wxiaoguang
Copy link
Copy Markdown
Contributor Author

wxiaoguang commented Sep 19, 2023 •
edited
Loading

But if assert.NoError fails, the test will already be marked as failed, correct?

Yes. But there could be no different no matter the "defer" comes before or after if there is no return, the code could still execute to defer nil.Close() if the assert fails.

I do not consider it as a serious problem in test code. But since 27129 has been merged, I think it's better to show the right approach.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Sep 19, 2023
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Sep 19, 2023
@puni9869 puni9869 added skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/testing reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Sep 19, 2023
@puni9869
Copy link
Copy Markdown
Member

puni9869 commented Sep 19, 2023

🚀

@KN4CK3R
Copy link
Copy Markdown
Member

KN4CK3R commented Sep 19, 2023

This code is cleaner than the panic but we would need to rewrite lots of tests. At the moment we don't evade panics and I see not much value in doing so. The code in this PR for example should check the returned repo with assert.NotNil too. Here we would need the return too. And for every slice length check and so on. That will just bloat our tests.

It would be better to use require which instantly fails and aborts execution of the test. But for that we still would need to rewrite lots of tests.

@KN4CK3R KN4CK3R removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 19, 2023
@wxiaoguang
Copy link
Copy Markdown
Contributor Author

wxiaoguang commented Sep 20, 2023

At the moment we don't evade panics and I see not much value in doing so.

Agree with it (also my comment #27139 (comment): I do not consider it as a serious problem in test code. But since 27129 has been merged, I think it's better to show the right approach.)

In some cases, if assert is helpful, while for other cases, it's safe to leave the panic in the test.

@wxiaoguang
Copy link
Copy Markdown
Contributor Author

wxiaoguang commented Sep 20, 2023

I think we can merged this. If it doesn't look good or need future improvements, feel free to revert and/or propose new fixes.

@wxiaoguang wxiaoguang merged commit 2814f2f into go-gitea:main Sep 20, 2023
@GiteaBot GiteaBot added this to the 1.22.0 milestone Sep 20, 2023
@wxiaoguang wxiaoguang deleted the fix-wiki-test branch September 20, 2023 00:51
zjjhot added a commit to zjjhot/gitea that referenced this pull request Sep 20, 2023
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
add8afe 
* giteaofficial/main:
  Improve actions docs related to `pull_request` event (go-gitea#27126)
  Remove outdated paragraphs when comparing Gitea Actions to GitHub Actions (go-gitea#27119)
  Fix: treat tab "overview" as "repositories" in user profiles without readme (go-gitea#27124)
  Fix incorrect test code for error handling (go-gitea#27139)
  Increase auth provider icon size on login page (go-gitea#27122)
  fix pagination for followers and following (go-gitea#27127)
  services/wiki: Close() after error handling (go-gitea#27129)
  Use fetch helpers instead of fetch (go-gitea#27026)
  Change green buttons to primary color (go-gitea#27099)
  Fix wrong xorm get usage on migration (go-gitea#27111)
  Fix the incorrect route path in the user edit page. (go-gitea#27007)
  Refactor lfs requests (go-gitea#26783)
  Display archived labels specially when listing labels (go-gitea#26820)
  Remove a `gt-float-right` and some unnecessary helpers (go-gitea#27110)
  [skip ci] Updated licenses and gitignores
  Fix token endpoints ignore specified account (go-gitea#27080)
  Make SSPI auth mockable (go-gitea#27036)
@delvh delvh modified the milestones: 1.22.0, 1.21.0 Sep 20, 2023
silverwind added a commit to silverwind/gitea that referenced this pull request Sep 21, 2023
@silverwind
Merge remote-tracking branch 'origin/main' into vitest-global
d8ee8ac 
* origin/main:
  Fix dropdown icon position (go-gitea#27175)
  Fix repo sub menu (go-gitea#27169)
  Fix review request number and add more tests (go-gitea#27104)
  Fix the variable regexp pattern on web page (go-gitea#27161)
  Fix organization field being null in POST /orgs/{orgid}/teams (go-gitea#27150)
  Add index to `issue_user.issue_id` (go-gitea#27154)
  [skip ci] Updated translations via Crowdin
  Start development on Gitea 1.22 (go-gitea#27155)
  Fix successful return value for `SyncAndGetUserSpecificDiff` (go-gitea#27152)
  Improve actions docs related to `pull_request` event (go-gitea#27126)
  Remove outdated paragraphs when comparing Gitea Actions to GitHub Actions (go-gitea#27119)
  Fix: treat tab "overview" as "repositories" in user profiles without readme (go-gitea#27124)
  Fix incorrect test code for error handling (go-gitea#27139)
  Increase auth provider icon size on login page (go-gitea#27122)
  fix pagination for followers and following (go-gitea#27127)
  services/wiki: Close() after error handling (go-gitea#27129)
  Use fetch helpers instead of fetch (go-gitea#27026)
  Change green buttons to primary color (go-gitea#27099)
  Fix wrong xorm get usage on migration (go-gitea#27111)
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Dec 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@delvh delvh delvh approved these changes

+1 more reviewer

@puni9869 puni9869 puni9869 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/testing

Projects

None yet

Milestone

1.21.0

Development

Successfully merging this pull request may close these issues.

5 participants

@wxiaoguang @delvh @puni9869 @KN4CK3R @GiteaBot